Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not ignore constructor arguments based on type being mocked #3694

Merged
merged 1 commit into from May 17, 2019
Merged

Do not ignore constructor arguments based on type being mocked #3694

merged 1 commit into from May 17, 2019

Conversation

morozov
Copy link
Contributor

@morozov morozov commented May 16, 2019

Currently, it is impossible to mock the message or code on a Trowable. On the one hand, getMessage() and getCode() are final in Exception, on the other, the mock generator ignores constructor arguments if the type being mocked is an interface.

The existing behavior doesn't seem correct because on the one hand, PHP currently allows passing more arguments than the method expects (including class constructors). On the other, if it stops doing so at some point later, it should be a test error, not the PHPUnit's call to ignore the arguments.

@codecov
Copy link

codecov bot commented May 16, 2019

Codecov Report

Merging #3694 into 7.5 will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##                7.5    #3694      +/-   ##
============================================
- Coverage     83.37%   83.36%   -0.01%     
+ Complexity     3647     3645       -2     
============================================
  Files           143      143              
  Lines          9713     9711       -2     
============================================
- Hits           8098     8096       -2     
  Misses         1615     1615
Impacted Files Coverage Δ Complexity Δ
src/Framework/MockObject/Generator.php 86.41% <100%> (-0.07%) 139 <0> (-2)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update de00854...278379a. Read the comment docs.

morozov added a commit to morozov/dbal that referenced this pull request May 16, 2019
1. Got rid of hard-coded exception mocks in favor of PHPUnit-generated ones.
2. Reworked AbstractDriverTest::testConvertsException() to use a data provider to make testing all types of exceptions independent.
3. Updated PHPUnit since the reworked code relies on sebastianbergmann/phpunit#3604 and sebastianbergmann/phpunit#3694.
morozov added a commit to morozov/dbal that referenced this pull request May 16, 2019
1. Got rid of hard-coded exception mocks in favor of PHPUnit-generated ones.
2. Reworked AbstractDriverTest::testConvertsException() to use a data provider to make testing all types of exceptions independent.
3. Updated PHPUnit since the reworked code relies on sebastianbergmann/phpunit#3604 and sebastianbergmann/phpunit#3694.
@sebastianbergmann
Copy link
Owner

Just curious: in what scenario does stubbing/mocking Throwable or Exception make sense? I have never though about this before the recent patches, much less needed it.

@sebastianbergmann sebastianbergmann merged commit b961563 into sebastianbergmann:7.5 May 17, 2019
@morozov
Copy link
Contributor Author

morozov commented May 17, 2019

In doctrine/dbal, we have some logic of handling/generalizing driver exceptions based on their properties. We do not mock Throwable or Exception directly but we do mock interface DriverException extends Throwable.

@morozov morozov deleted the throwable-ctr-args branch May 17, 2019 05:06
@sebastianbergmann
Copy link
Owner

Can you point me to a test that does this? Thanks!

@morozov
Copy link
Contributor Author

morozov commented May 17, 2019

See Doctrine\Tests\DBAL\Driver\AbstractDriverTest::testConvertsException() for example. Right now, we implement the mock using an anonymous class but I'd like to rework it using PHPUnit mocks in doctrine/dbal#3546.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants